Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

staketia migration - staketia accounting #1214

Merged
merged 10 commits into from
Jun 7, 2024

Conversation

sampocs
Copy link
Collaborator

@sampocs sampocs commented Jun 4, 2024

Context

This PR handles the main accounting changes that will take place in the staketia module during the migration. At a high level, the main changes are:

  • Staketia now reads the redemption rate from stakeibc
  • The DelegatedBalance field on the host zone is now renamed to RemainingDelegatedBalance and is only meant to track the delegations left in the multisig account.

Context on Accounting

In staketia, the DelegatedBalance field previously was changed in the following stages:

  • LiquidStake - no change
  • ConfirmDelegation - incremented
  • RedeemStake - no change
  • ConfirmUndelegation - decremented

And in stakeibc, we similarly have the following with the TotalDelegations field on host zone

  • LiquidStake - no change
  • DelegationCallback - incremented
  • RedeemStake - no change
  • UndelegationCallback - decremented

After the migration, the replacement field RemainingDelegatedBalance in staketia will update as follows:

  • LiquidStake - no change (obviously)
  • ConfirmDelegation - incremented (only occurs in one delegation cycle after the upgrade - see phase 2.5 in spec)
  • RedeemStake - decremented so that we know when to switch over after the last token's been redeemed
  • ConfirmUndelegation - no change
    • However TotalDelegations in stakeibc will be decremented

Accounting Changelog

Based on the above, there are the following changes:

  • ConfirmDelegation now increments RemainingDelegatedBalance instead of DelegatedBalance
  • RedeemStake (handled in separate PR)
  • ConfirmUndelegation now decrements TotalDelegations instead of DelegatedBalance
  • AdjustDelegatedBalance now references RemainingDelegatedBalance instead of DelegatedBalance

Remaining Changelog

  • Renamed DelegatedBalance to RemainingDelegatedBalance on host zone
  • Removed redemption rate from host zone
  • Removed code that updates the redemption rate, as well as the redemption rate invariant check
  • Read redemption rate from stakeibc in relevant places

@sampocs sampocs marked this pull request as ready for review June 7, 2024 01:54
@sampocs sampocs merged commit aa25564 into staketia-migration Jun 7, 2024
1 check passed
@riley-stride
Copy link
Contributor

riley-stride commented Jul 22, 2024

How do we handle rate limits when migrating the accounting / RR - is removing the checks from staketia enough, since ratelimiting covers stakeibc?

// If this fails, do not proceed to the delegation or undelegation step
// Note: This must be run first because it is used when refreshing the native token
// balance in prepare undelegation
if err := k.UpdateRedemptionRate(ctx); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that the ordering of refreshing the RR and sending delegations and undelegations matters. The comments here suggest the same.

If we move the update RR step to stakeibc, but keep the delegation and undelegation txs in staketia, we just need to make sure the update RR is run at the same epochly cadence.

Just to triple check - it is, right? I.e. the cadence at which we ran it here matches the cadence at which we run it in stakeibc?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GREAT question! The cadence will be different in stakeibc (it runs every stride epoch instead of every day epoch)

i think how we have it right now, the RR will be ~6 hours stale when its used for unbondings here. But imo, since that's pretty negligible, I'd prefer to keep it as is, rather than mess with the ordering. Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I see.

So just to map out the worst case: some packets may be are stuck or reinvestment would be otherwise delayed for a long time, then we'd see a large RR jump that triggers within the "stale period". In that case, we'd use the stale RR for unbonding and the users who receive those unbondings would not receive their yield from the "stuck packet" period. Is that right?

Just to explore syncing up the epochs.... what if we had staketia epochs run every 6 hours? I imagine not much would change in the 6-hourly logic until the operator advances the state. But in that case, would this edge case or the "stale period" be solved?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think that worst case is righ!, But I think it's pretty unlikely that we have some multi-day bottleneck that just happens to clear in this 6 hour window. And even in that case, the impact is low - they just get the RR at the time of redemption (similar to how it used to be done).

Running the epochs every 6 hours could work, but I'd be a bit worried we have some implicit invariant about the length and not sure it's worth the effort to investigate based on the above

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

even in that case, the impact is low - they just get the RR at the time of redemption (similar to how it used to be done).

Yeah good point that it'd revert to how it used to be done. We did get some complaints about that, but as long as we don't have a long packet backlog we're good. And ultimately this migration period is temporary so the time window within which we could see backlogs is bounded.

Fwiw I'm mostly convinced to leave things as is, especially because of "we [may] have some implicit invariant about the length" could lead to a serious accounting issue if overlooked.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good point about this being temporary!

also I assume the complaints were more in the case where the unbondings were delayed right? I’d imagine w/o a delay, getting the same RR that’s shown in the FE at time of redemption wouldnt upset anyone!

Copy link
Contributor

@riley-stride riley-stride left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking really clean. PR description and spec helped a ton in the review, ty for making those to clear. Left a few questions.

@riley-stride
Copy link
Contributor

How do we handle rate limits when migrating the accounting / RR - is removing the checks from staketia enough, since ratelimiting covers stakeibc?

@sampocs i think this question is still outstanding

@sampocs
Copy link
Collaborator Author

sampocs commented Aug 7, 2024

We don't have any tia RLs at the moment. Also RLs are module agnostic!

@riley-stride
Copy link
Contributor

I'm signed off here. Leaving this open for other reviewers to opine on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants